Replace alert() in preview with non-blocking sweetalert #157
Conversation
alexpelan
force-pushed
the
sweetalert
branch
2 times, most recently
from
88409d3 to
48701b3
Compare
|
Sick!!! |
Given that, should we ditch sweetalert and make |
README.md
Outdated
| That'll pull down the dependencies. | ||
|
|
||
| ```bash | ||
| $ bower install |
There was a problem hiding this comment.
Surprised this was necessary, actually—bower install should be a postinstall hook…
There was a problem hiding this comment.
Hmm, I see it there - but I ran bower install, not the version installed in gulp - so maybe it was a bower version mismatch on my machine or something? I'll remove it til we see if other folks have problems.
|
@alexpelan this is bomb!! left a bunch of comments, all just starting points for discussion |
|
I don't think we should ditch them for no-op - I think it's useful feedback in preview to see the alerts showing up / changing while you are coding. Since prompt isn't addressed by this PR, maybe just no-op it for now? |
|
Ok, ready for another look if you want to. I tried just 'npm shrinkwrap' but it looked like that added a bunch of artifacts, and wiped out the node version and stuff - so let me know if there are any instructions to follow to do that right, and if i need a certain node/npm version. |
|
@alexpelan ah, yes, you’ll want |
src/components/Preview.jsx
Outdated
| } | ||
|
|
||
| _generateDocument() { | ||
| _generateDocument(isPopout = false) { |
There was a problem hiding this comment.
Any reason to take this as a parameter, since it’ll always be true in the context of the Preview component?
There was a problem hiding this comment.
WAIT I’m a moron.
We should actually be taking this param and using it for all the options being passed to generatePreview below.
Still stand by the idea of keeping the popout semantics confined to this component tho
There was a problem hiding this comment.
Not sure I follow - the popout action calls this with true, the in window preview calls it with false. The other 3 parameters should always be true, or at least that seems to be the behavior before this change that I'm making.
There was a problem hiding this comment.
@alexpelan right, sorry, that was confusing. my first comment was just wrong. second comment is basically saying that the preexisting code is also wrong—we should be varying the values of all the options passed to _generatePreview from the _generateDocument method. However I am down to address that separately since it’s outside the scope of this change. Making a ticket now!
|
Alright - there was one comment of yours I wasn't sure about but hopefully we're almost ready to merge. |
src/util/generatePreview.js
Outdated
|
|
||
| Object.keys(alwaysOnLibraries).forEach((libraryKey) => { | ||
| const library = alwaysOnLibraries[libraryKey]; | ||
| this._attachLibrary(library); |
There was a problem hiding this comment.
Ack, sorry, I didn’t think of this before, but: I think we only want to load sweetalert if we’re also adding alert handling (i.e. neither should exist in the pop-out environment). So, rather than having the concept of “always on”, I think it’s more a situation of “internal libraries that are available to generatePreview to use as needed”. This makes the _attachLibrary method even more useful, and also cuts in favor of the approach here you took of keeping sweetalert separate from the user-toggled libraries. Thoughts?
|
Ok, it seemed like that was the only thing to address, and we were gonna leave the comment about the different option parameters passed to generatePreview to another change set. I was kinda assuming you'd squash all these before merge, but let me know - however you want your git history, I can make it so. |
|
Sick! Yes! Let’s do this! |
Also, introduce a new configuration file like libraries.js that is used for libraries that we might want to inject into the preview frame, like sweetalert. We don't want these in the popout, which uses native alert.
|
Done |
|
Here goooooooes |
2 participants
The sweetalert functionality is...fairly meh. Since it doesn't block, if there are multiple alerts, then it just renders the last one as a preview. And we can't replace prompt with it. We could do a lot more by patching it down the line, but it would take some dev effort to have the user writing synchronous, blocking code and us transforming it to be asynchronous with this stuff.